Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove source_span_untracked #108034

Closed
wants to merge 1 commit into from
Closed

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 14, 2023

it only has one caller, and we can avoid query dep tracking and still call the query

r? @ghost (I want to perf test this first)

it only has one caller, and we can avoid query dep tracking and still call the query
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 14, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 14, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2023
@bors
Copy link
Contributor

bors commented Feb 14, 2023

⌛ Trying commit db2679d with merge c2c12d8821286c83f8a6be184c16637b4f1a4327...

@bors
Copy link
Contributor

bors commented Feb 14, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 14, 2023

random github/network failure

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 14, 2023

⌛ Trying commit db2679d with merge 49fba494c1bcbeab1e5159e294496594dc21fa56...

@bors
Copy link
Contributor

bors commented Feb 14, 2023

☀️ Try build successful - checks-actions
Build commit: 49fba494c1bcbeab1e5159e294496594dc21fa56 (49fba494c1bcbeab1e5159e294496594dc21fa56)

@rust-timer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Attempting with retry: docker build --rm -t rust-ci -f /home/runner/work/rust/rust/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile /home/runner/work/rust/rust/src/ci/docker
Sending build context to Docker daemon  688.6kB

Step 1/27 : FROM centos:7
Get "https://registry-1.docker.io/v2/": EOF
Sending build context to Docker daemon  688.6kB

Step 1/27 : FROM centos:7
Step 1/27 : FROM centos:7
Get "https://registry-1.docker.io/v2/": EOF
Sending build context to Docker daemon  688.6kB

Step 1/27 : FROM centos:7
Step 1/27 : FROM centos:7
Get "https://registry-1.docker.io/v2/": EOF
Sending build context to Docker daemon  688.6kB

Step 1/27 : FROM centos:7
Step 1/27 : FROM centos:7
Get "https://registry-1.docker.io/v2/": EOF
Sending build context to Docker daemon  688.6kB

Step 1/27 : FROM centos:7
Step 1/27 : FROM centos:7
Get "https://registry-1.docker.io/v2/": EOF
##[error]Process completed with exit code 1.
Post job cleanup.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (49fba494c1bcbeab1e5159e294496594dc21fa56): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.2%, 0.8%] 12
Regressions ❌
(secondary)
0.7% [0.4%, 1.0%] 10
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.2%, 0.8%] 12

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 14, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 14, 2023

The additional query call has a noticeable performance regression for decoding datastructures that contain a Span. @Zoxc you recently touched queries with () keys, do you have any ideas for more avenues to move the perf of a ()-keyed query closer to a direct member access while ignoring query dependencies? Or just ideas how to even drill into where the time is spent?

14,942,026  ???:<alloc::vec::Vec<rustc_middle::mir::Statement> as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode
11,534,182  ???:<rustc_middle::mir::Constant as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode
 5,898,265  ???:<rustc_middle::ty::Ty as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode
 5,505,023  ???:<rustc_middle::mir::LocalDecl as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode
 2,097,121  ???:<(rustc_middle::mir::syntax::Place, rustc_middle::mir::syntax::Rvalue) as rustc_data_structures::stable_hasher::HashStable<rustc_query_system::ich::hcx::StableHashingContext>>::hash_stable

@Zoxc
Copy link
Contributor

Zoxc commented Feb 14, 2023

An query accessor optimized for non-tracked access wouldn't be hard to add, however this encoding scheme with internal query usage does seem quite questionable. The current scheme does seem good for performance and if the goal is to move source_span to a query you'd get it down to a branch for an optimized untracked () key.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 14, 2023

if the goal is to move source_span to a query

yea. Ideally we'd move the resolver into a query and return the Definitions, CStore and SourceSpan from that query. The other alternative is to mutate these three fields from within the resolver (this already happens for Definitions, can easily be done for SourceSpan, but is expensive and/or annoying for CStore. I did the latter experiment in #105462

@Zoxc
Copy link
Contributor

Zoxc commented Feb 14, 2023

We should probably check that source_span has been marked red or green before accessing it in the decoder. This ensures that the span we decode has some data dependency on source_span.

@oli-obk oli-obk closed this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants